-
Notifications
You must be signed in to change notification settings - Fork 532
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Store handles in detached DDS before attaching #21132
Conversation
packages/test/test-end-to-end-tests/src/test/handleValidation.spec.ts
Outdated
Show resolved
Hide resolved
packages/test/test-end-to-end-tests/src/test/handleValidation.spec.ts
Outdated
Show resolved
Hide resolved
const attachedDataStore = | ||
(await container1.getEntryPoint()) as ITestFluidObject; | ||
|
||
const dataStoreB = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i don't think we want a new data store here and we should just use the attachedDataStore's runtime. The case with a new datastore is interesting, but i think that is covered in the pervious, at least to some degree. The differenence is if the to whole data store is detached, it will get one big summary of the whole datastore and send an attach op for that, but if the datastore is already attached, then each dds will get its own attach op.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something interesting about this - with the new data store, the majority of the test cases fail, except for the ones that use map and directory (the ones that have the bindHandles workaround for AB#7452). Without this data store, they all pass, but I'm not sure if we still need coverage for not binding handles from all of the other DDSs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
interesting. it might be useful to have both tests then, those that use a new datastore, and those that do not.
await dataStore.getSharedObject<IConsensusRegisterCollection<FluidObject>>( | ||
registerId, | ||
); | ||
return this.downCast(register); | ||
}, | ||
}, | ||
// { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ConsensusQueue has an issue that causes the tests to hang. I'm looking into it, but commented it out for now.
if (!(context instanceof LocalChannelContextBase)) { | ||
throw new LoggingError("Should only be called with local channel handles"); | ||
} | ||
const visitedContexts = new Set<string>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@agarwal-navin: @anthony-murphy and I were wondering if this is an ok way to solve the problem of contexts for handles stored in other detached DDSs not being seen. The goal is to ensure that all contexts, even those for handles stored in a detached DDS, are able to be resolved and the handles will still get bound.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this solve the problem where binding a context results in another context binding, which the current code can miss, if the content that gets bound is before the context that binds it in the contexts list
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it would be good to get this in and start back porting to rc3 and rc4. we may find a better way to do this, but i think we can forward fix in main if we need to
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jzaffiro Can you explain the bug to me that you are trying to fix? The description says this is adding tests but it has code changes as well so I am not clear what is being solved here :-).
Is the problem only what Tony pointed out - "if the content that gets bound is before the context that binds it in the contexts list"? Basically, due to ordering we can miss some contexts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tests are to make sure we don't hit a bug where handles that are stored in detached DDSs are not bound when we attach. The tests starting on line 501 of handleValidation.spec.ts were not able to find the DDS referenced to in the other detached DDS, which is what this code here solves. I'll update the description as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before #20998, we were also seeing a bug where going from detached --> attached would cause duplicate attach ops to be sent, and the tests starting on line 405 make sure we have coverage for all DDSs for that bug
⯅ @fluid-example/bundle-size-tests: +222 Bytes
Baseline commit: 1a92d06 |
Add more tests that involve handles stored in a detached DDS attaching due to the storing of a handle of one of the detached DDSs in an attached DDS. In addition, fixes a bug where some of the contexts were not bound based on the ordering of contexts in the context list. [AB#8009](https://dev.azure.com/fluidframework/235294da-091d-4c29-84fc-cdfc3d90890b/_workitems/edit/8009) --------- Co-authored-by: Tony Murphy <anthonm@microsoft.com>
Add more tests that involve handles stored in a detached DDS attaching due to the storing of a handle of one of the detached DDSs in an attached DDS. In addition, fixes a bug where some of the contexts were not bound based on the ordering of contexts in the context list. [AB#8009](https://dev.azure.com/fluidframework/235294da-091d-4c29-84fc-cdfc3d90890b/_workitems/edit/8009) --------- Co-authored-by: Tony Murphy <anthonm@microsoft.com>
…1188) Add more tests that involve handles stored in a detached DDS attaching due to the storing of a handle of one of the detached DDSs in an attached DDS. In addition, fixes a bug where some of the contexts were not bound based on the ordering of contexts in the context list. Port of #21132 [AB#8009](https://dev.azure.com/fluidframework/235294da-091d-4c29-84fc-cdfc3d90890b/_workitems/edit/8009) Co-authored-by: Tony Murphy <anthonm@microsoft.com>
…1187) Add more tests that involve handles stored in a detached DDS attaching due to the storing of a handle of one of the detached DDSs in an attached DDS. In addition, fixes a bug where some of the contexts were not bound based on the ordering of contexts in the context list. Port of #21132 [AB#8009](https://dev.azure.com/fluidframework/235294da-091d-4c29-84fc-cdfc3d90890b/_workitems/edit/8009) --------- Co-authored-by: Tony Murphy <anthonm@microsoft.com>
Add more tests that involve handles stored in a detached DDS attaching due to the storing of a handle of one of the detached DDSs in an attached DDS. In addition, fixes a bug where some of the contexts were not bound based on the ordering of contexts in the context list.
AB#8009